@Atemu as discussed in #339370

Inheriting meta whole is generally not a good idea. Always pick the parts you actually care about.

That does sound reasonable, but cc-wrapper does:

  meta =
    let cc_ = optionalAttrs (cc != null) cc; in
    (optionalAttrs (cc_ ? meta) (removeAttrs cc.meta ["priority"])) //
    { description = attrByPath ["meta" "description"] "System C compiler" cc_ + " (wrapper script)";
      priority = 10;
      mainProgram = if name != "" then name else "${targetPrefix}${ccName}";
  };

So it only replaces those three attributes and inherits everything else.

What I've done here only sets description, maintainers (i.e. the maintainer of the wrapper / combiner), and mainProgram.

Edit: platforms as well, since it's required for buildDotnetModule.

swift-wrapper inherits all of meta:

stdenv.mkDerivation (swift._wrapperParams // {
  pname = "swift-wrapper";
  inherit (swift) version meta;

pkgconfig-wrapper does something like cc-wrapper:

  meta =
    let pkg-config_ = optionalAttrs (pkg-config != null) pkg-config; in
    (optionalAttrs (pkg-config_ ? meta) (removeAttrs pkg-config.meta ["priority" "mainProgram"])) //
    { description =
        attrByPath ["meta" "description"] "pkg-config" pkg-config_
        + " (wrapper script)";
      priority = 10;
      mainProgram = wrapperBinName;
  };

rustc-wrapper:

  meta = rustc-unwrapped.meta // {
    description = "${rustc-unwrapped.meta.description} (wrapper script)";
    priority = 10;
  };

Yes, it doesn't, but it gets used as reference for new things constantly and provides a certain measure of standardization as if you've seen one wrapper you have an idea of how other wrappers will work. If this wasn't an issue for other compilers which are used much more widely than .NET I don't see why this should be an issue for us or something addressed at a higher level across all of nixpkgs.

Users are putting themselves and others at risk by using .NET 6 and/or 7. I don't think this should be easy nor painless to do. It should cause as big of an annoyance as possible so they actually think about what they're doing and maintainers actually do something about it.

Lets discuss this here because it's not related to the line of code quoted.

This is about whether we do anything at all. i.e. Do we even remove knownVulnerabilities from the wrapper?

Right now you have to do this:

    nixpkgs.config.permittedInsecurePackages = [
      "dotnet-sdk-6.0.428"
      "dotnet-sdk-wrapped-6.0.428"
    ];

IMO there's no benefit in requiring that the wrapper be permitted separately. It's just confusing.

IMO there's no benefit in requiring that the wrapper be permitted separately. It's just confusing.

Yes, I don't disagree it's confusing, however we're causing maintainer churn for something that'll ultimately be removed (.NET 6 and 7).
We're putting effort into something that doesn't really benefit anyone, my issue here is that we're focusing on things that shouldn't really be relevant.

It benefits me because I currently have all of this in my local config:

            permittedInsecurePackages = map (x: x.name) (with pkgs; [
              dotnet-sdk_6
              dotnet-sdk_6.unwrapped
              dotnet-sdk_7
              dotnet-sdk_7.unwrapped
              dotnet-runtime_6
              dotnet-runtime_6.unwrapped
              dotnet-runtime_7
              dotnet-runtime_7.unwrapped
            ]) ++ [
              "dotnet-core-combined"
            ];

and I'm not going to realistically stop using those packages any time soon.

It benefits me because I currently have all of this in my local config:

            permittedInsecurePackages = map (x: x.name) (with pkgs; [
              dotnet-sdk_6
              dotnet-sdk_6.unwrapped
              dotnet-sdk_7
              dotnet-sdk_7.unwrapped
              dotnet-runtime_6
              dotnet-runtime_6.unwrapped
              dotnet-runtime_7
              dotnet-runtime_7.unwrapped
            ]) ++ [
              "dotnet-core-combined"
            ];

and I'm not going to realistically stop using those packages any time soon.

True, but is all of this worth it to avoid adding just one extra like per SDK?
This feels like too much work considering all the decision making and bikeshedding we're running into.

@Atemu continuing here since it's unrelated to maintainers line.

Because dotnet is the only compiler I know of in Nixpkgs that gets EOL'd via insecure.

Maybe we should look at how java handles this? Considering they have the same model there (compiler + runtime).

Btw, why is the compiler EOL'd anyways? Isn't only the runtime relevant for security? Not to deep into dotnet to know the details here.

The SDK could be left as not insecure, however the packages used at runtime definitely should be marked as such if we do so. As a package can compile with a more recent SDK and run in a more recent runtime runtime but use packages from an earlier release which are EOL and possibly insecure.
And using the compiler itself, one can also abuse vulnerabilities in the compiler itself and its tooling to abuse security vulnerabilities, I don't know if that counts.

Having to put it in permittedInsecure is annoyance enough. Anything past that is extremely unnecessary.

Yes, but if we marked them as unsupported I don't see why we should waste extra effort on it as we'll be removing it once it's no longer used in nixpkgs anyways.
And considering the incoming change of marking all of .NET 6 and 7 packages as insecure, the annoyance will become even bigger with needing to add 10 to 30 lines in it.
If someone or their project are still stuck in an unsupported version of .NET, that's a them problem and it shouldn't be up to nixpkgs to make their life easier to use something that's no longer supported. We've done our part notifying them that it's dead and the rest is up to them to solve, I don't think we should waste our time on it.

Maybe we should look at how java handles this? Considering they have the same model there (compiler + runtime).

Java apps generally depend on LTS versions and those are generally maintained for decades. It's rare for a Java app that is not compatible with a supported LTS JDK to have any relevance whatsoever because it must have been last updated well over a decade ago.

I'm not saying this doesn't happen but it's exceedingly rare I think.

if we marked them as unsupported I don't see why we should waste extra effort on it as we'll be removing it once it's no longer used in nixpkgs anyways.

To not make the lives of those who have no other choice unnecessarily hard.

considering the incoming change of marking all of .NET 6 and 7 packages as insecure

I'm not quite sure what you mean by this? Marking the SDK and runtimes as insecure has already happened which has already marked all dependent packages as insecure transitively.

Are you intending on marking the individual packages as insecure?

If so, why? That wouldn't solve any problem and only cause unnecessary pain. It'd also be wrong because the packages themselves don't have security issues, it's the SDKs they use and those are already marked appropriately and cause an eval abort.

If someone or their project are still stuck in an unsupported version of .NET, that's a them problem and it shouldn't be up to nixpkgs to make their life easier to use something that's no longer supported.

Again, this isn't about making their life easier through significant effort, it's about not making their lives unnecessary hard with very little effort in correctly setting metadata. It's simply incorrect to mark i.e. the wrapper as insecure because the wrapper code itself doesn't have any known security issue whatsoever, only the wrapped SDK.

Are you intending on marking the individual packages as insecure?

Yes.

If so, why? That wouldn't solve any problem and only cause unnecessary pain.

It would, because if you look at the other PR, that removes unnecessary uses of combinePackages, references to the older SDKs are being removed and only their packages kept which aren't marked as insecure currently.

It'd also be wrong because the packages themselves don't have security issues, it's the SDKs they use and those are already marked appropriately and cause an eval abort.

It wouldn't be wrong. Many .NET CVEs have been causes by packages and not the runtime itself. Including RCEs. Just look through previous advisories and you'll see they mention package versions and not the runtime itself.

For example, System.Text.Json which is part of the SDK/runtime packages: dotnet/announcements#329

Again, this isn't about making their life easier through significant effort, it's about not making their lives unnecessary hard with very little effort in correctly setting metadata. It's simply incorrect to mark i.e. the wrapper as insecure because the wrapper code itself doesn't have any known security issue whatsoever, only the wrapped SDK.

Fair, but the eval error already tells the person everything they have to add, is it really making their lives harder if we spoonfeed them everything they need to do to fix it?

I'm not entirely against fixing this, I'm just upset at this being solved here and not at a higher level with guidance on the best way to do this considering that much larger packages do this and don't seem to be doing any similar changes.

Are you intending on marking the individual packages as insecure?

Yes.

I may be wrong, but I think you're talking about different things.

@GGG-KILLER is talking about marking the nuget packages associated with an insecure SDK/runtime insecure, not marking packages built with the SDK insecure.

So for example:

      (fetchNupkg { pname = "Microsoft.NETCore.App.Runtime.linux-x64"; version = "6.0.36"; hash = "sha512-QOBu74LANKrjgQI6NMSSHZkungzGW+79tlOSv04HuGkphqCf330pMeCFvQn7U/i+egShqpFVZiFZDuzoTnCNjg=="; })

However, I think this is getting off topic for this PR. We'll do another for that change, and we can discuss it there.

@GGG-KILLER @Atemu Do either of you (or anyone else) have any actual problems with this PR as it is?

This feels like too much work considering all the decision making and bikeshedding we're running into.

IMO most of this has been off-topic.

Do either of you (or anyone else) have any actual problems with this PR as it is?

I have a problem with the maintainers list, lack of url and homepage.
URL and homepage show on search.nixos.org so it's a nice to have and the maintainers should preserve the original maintainers in case the problem is with the wrapped package, not the wrapper imo.
The license field is also missing which prevents Hydra from caching the build results so it's important to have.

For reference:

{
  available = true;
  broken = false;
  description = ".NET SDK 9.0.100";
  homepage = "https://dotnet.github.io/";
  insecure = false;
  knownVulnerabilities = [ ];
  license = {
    deprecated = false;
    free = true;
    fullName = "MIT License";
    redistributable = true;
    shortName = "mit";
    spdxId = "MIT";
    url = "https://spdx.org/licenses/MIT.html";
  };
  mainProgram = "dotnet";
  maintainers = [
    {
      email = "[email protected]";
      github = "kuznero";
      githubId = 449813;
      name = "Roman Kuznetsov";
    }
    {
      email = "[email protected]";
      github = "mdarocha";
      githubId = 11572618;
      name = "Marek Darocha";
    }
    {
      email = "[email protected]";
      github = "corngood";
      githubId = 3077118;
      name = "David McFarland";
    }
  ];
  name = "dotnet-sdk-9.0.100";
  outputsToInstall = [ "out" ];
  platforms = [
    "x86_64-darwin"
    "aarch64-darwin"
    "aarch64-linux"
    "x86_64-linux"
  ];
  position = "/home/david/src/nixpkgs/pkgs/development/compilers/dotnet/build-dotnet.nix:212";
  sourceProvenance = [
    {
      isSource = false;
      shortName = "binaryBytecode";
    }
    {
      isSource = false;
      shortName = "binaryNativeCode";
    }
  ];
  unfree = false;
  unsupported = false;
}

So how about we keep:

• maintainers
• platforms
• description (with suffix?)
• homepage

The license field is also missing which prevents Hydra from caching the build results so it's important to have.

I'm not sure that's the case. Things that live in nixpkgs (e.g. stdenv, nuget-to-nix) seem to omit license and are still cached.

So how about we keep:

• maintainers
• platforms
• description (with suffix?)
• homepage

Sounds good to me!

The license field is also missing which prevents Hydra from caching the build results so it's important to have.

I'm not sure that's the case. Things that live in nixpkgs (e.g. stdenv, nuget-to-nix) seem to omit license and are still cached.

But won't that affect the display in search.nixos.org as well?
It's important to not omit information from the end user of the package being wrapped. Ideally the wrapper should be totally invisible to the end user.

But won't that affect the display in search.nixos.org as well?

Yes, presumably. I think that's a good enough reason to include license as well.

Okay, I've updated it as discussed. Here's a diff of the wrapped meta attrset:

2c2
<   available = false;
---
>   available = true;
4c4
<   description = ".NET SDK 6.0.428";
---
>   description = ".NET SDK 6.0.428 (wrapper)";
6,7c6
<   insecure = true;
<   knownVulnerabilities = [ "Dotnet SDK 6.0.428 is EOL, please use 8.0 (LTS) or 9.0 (Current)" ];
---
>   insecure = false;
38c37
<   name = "dotnet-sdk-6.0.428";
---
>   name = "dotnet-sdk-wrapped-6.0.428";
46,56c45
<   position = "/home/david/src/nixpkgs/pkgs/development/compilers/dotnet/build-dotnet.nix:212";
<   sourceProvenance = [
<     {
<       isSource = false;
<       shortName = "binaryBytecode";
<     }
<     {
<       isSource = false;
<       shortName = "binaryNativeCode";
<     }
<   ];
---
>   position = "/home/david/src/nixpkgs/pkgs/development/compilers/dotnet/wrapper.nix:28";

and a combinePackages meta:

64c53
<   description = ".NET SDK 9.0.101";
---
>   description = ".NET SDK 9.0.101 (wrapper) (combined) (wrapper)";
67d55
<   knownVulnerabilities = [ ];
98c86
<   name = "dotnet-sdk-9.0.101";
---
>   name = "dotnet-wrapped-combined";
106,116c94
<   position = "/home/david/src/nixpkgs/pkgs/development/compilers/dotnet/build-dotnet.nix:212";
<   sourceProvenance = [
<     {
<       isSource = false;
<       shortName = "binaryBytecode";
<     }
<     {
<       isSource = false;
<       shortName = "binaryNativeCode";
<     }
<   ];
---
>   position = "/home/david/src/nixpkgs/pkgs/development/compilers/dotnet/wrapper.nix:28";

(wrapper) (combined) (wrapper)

Yes, this is weird, but it is technically a wrapper around the combination of some wrappers. It's also unlikely that a user will ever see the meta of the output of combinePackages.

Not ideal but looks good enough to me. Preferably we wouldn't have that many suffixes but the end user probably won't see it as you mentioned.

I'm converting this back to a draft because I've added 6693d97, which is meant to mark the nuget packages as insecure along with the corresponding SDK.

It does this by requiring evaluation of the SDK, which means:

• the package derivations don't have to change
• permitting the sdk is enough to allow all the packages

Edit: this could be a separate PR, but it seems closely related, so I put it here for now.

builtins.seq finalAttrs.finalPackage.drvPath is how it accomplishes this. It's a little unusual, which makes me slightly nervous, but so far it seems to work. I tried making an actual separate pseudo-package for this purpose and adding it to buildInputs, but that results in the derivations changing for no good reason.

I'm converting this back to a draft because I've added 6693d97, which is meant to mark the nuget packages as insecure along with the corresponding SDK.

It does this by requiring evaluation of the SDK, which means:

• the package derivations don't have to change
• permitting the sdk is enough to allow all the packages

Edit: this could be a separate PR, but it seems closely related, so I put it here for now.

Interesting solution, wouldn't have thought of it myself!

builtins.seq finalAttrs.finalPackage.drvPath is how it accomplishes this. It's a little unusual, which makes me slightly nervous, but so far it seems to work. I tried making an actual separate pseudo-package for this purpose and adding it to buildInputs, but that results in the derivations changing for no good reason.

It is indeed unusual and I think it might be confusing for the maintainers of other packages that now only reference packages or targetPackages from an SDK due to the other PR, but that's something we can "hide" behind a helper later on if it gets confusing.

It seems pretty intuitive. It may not be clear exactly where the dependency comes from (it usually isn't anyway), but you still get the usual message, and the resolution is the same:

$ nix eval -f. roslyn-ls

«error: Package ‘dotnet-sdk-6.0.428’ in /home/david/src/nixpkgs/pkgs/development/compilers/dotnet/build-dotnet.nix:220 is marked as insecure, refusing to evaluate.


Known issues:
 - Dotnet SDK 6.0.428 is EOL, please use 8.0 (LTS) or 9.0 (Current)

You can install it anyway by allowing this package, using the
following methods:

a) To temporarily allow all insecure packages, you can use an environment
   variable for a single invocation of the nix tools:

     $ export NIXPKGS_ALLOW_INSECURE=1

   Note: When using `nix shell`, `nix build`, `nix develop`, etc with a flake,
         then pass `--impure` in order to allow use of environment variables.

b) for `nixos-rebuild` you can add ‘dotnet-sdk-6.0.428’ to
   `nixpkgs.config.permittedInsecurePackages` in the configuration.nix,
   like so:

     {
       nixpkgs.config.permittedInsecurePackages = [
         "dotnet-sdk-6.0.428"
       ];
     }

c) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
   ‘dotnet-sdk-6.0.428’ to `permittedInsecurePackages` in
   ~/.config/nixpkgs/config.nix, like so:

     {
       permittedInsecurePackages = [
         "dotnet-sdk-6.0.428"
       ];
     }

»
$ nix eval -f. roslyn-ls --arg config '{ permittedInsecurePackages = [ "dotnet-sdk-6.0.428" ]; }'

«derivation /nix/store/bkbizsb5g2iqca6ikl9k842v7nqfd433-roslyn-ls-4.13.0-3.24577.4.drv»

Fair enough, I'm really fond of this solution since it achieves the evaluation error I wanted and as a bonus doesn't generate a billion lines of permittedInsecurePackages.
Thank you for this wonderful solution!

--------- Impacted packages on 'x86_64-linux' ---------
2 packages removed:
godot4-mono (†4.3-stable) roslyn-ls (†4.13.0-3.24577.4)

These are the packages that become insecure with this change.

Thanks!

Successfully created backport PR for release-24.11:

• [Backport release-24.11] dotnet: improve EOL evaluation errors #364071